fix(disk-hygiene): stop guard hook fail-open on skill-hook data-root token - #1014

Claude finished @kyle-sexton's task in 4m 49s —— View job

Security Review — PR #1014

• Gather context
• Read changed files
• Analyze destructive_guard.py for path traversal and injection issues
• Analyze SKILL.md hook configuration for launch-path security
• Analyze safety-model.md for doc accuracy vs implementation
• Analyze test_hygiene.py for test-coverage gaps
• Post security findings

Summary

The core fix is sound and closes a genuine fail-open: ${CLAUDE_PLUGIN_DATA} was silently rejected by the skill-hook launcher, leaving the destructive-action guard unexecuted. Replacing it with ${CLAUDE_PLUGIN_ROOT} is the correct response, and the fail-closed behaviour for unrecognised layout paths is implemented correctly. One IMPORTANT defence-in-depth gap was found in the new derivation function.

Findings

IMPORTANT — _plugin_data_root_from_root() accepts relative paths and would return a relative authority

Confidence: CONFIRMED (the missing check is definitively observable; exploitability requires a non-absolute ${CLAUDE_PLUGIN_ROOT})

destructive_guard.py line 136

root = Path(plugin_root)
parents = root.parents
if len(parents) < 3:
    return None
marketplace_dir, cache_dir, plugins_dir = parents[0], parents[1], parents[2]

A relative path such as plugins/cache/m/n passes every layout check:

• parents[1].name = "cache" ✓
• parents[2].name = "plugins" ✓

The function therefore returns "plugins/data/n-m" — a relative path. When _is_authorized_data_root() later calls _data_root_key() → Path(value).expanduser().resolve(strict=False) on that relative authority, it resolves against the guard process's cwd rather than any intentional location. A model-provided --data-root pointing to the same cwd-relative location would pass the check, granting writes outside the plugin data directory.

In the current deployment this requires ${CLAUDE_PLUGIN_ROOT} to be non-absolute (Claude Code always provides absolute paths and the model cannot inject into hook args), so direct exploitability is low. However, _plugin_data_root_from_root() is intended to be a security boundary — the defence should be in code, not in a runtime guarantee external to the function. The test suite does not cover relative inputs.

Recommended fix: add an early return at the top of the function:

if not os.path.isabs(plugin_root):
    return None

And add a corresponding test case alongside the existing stray list:

self.assertIsNone(guard._plugin_data_root_from_root("plugins/cache/m/n"))

Fix this →

No other security issues were found. In particular:

• The _SHELL_EXPANSION_OR_OPERATOR_CHARS rejection and the _literal_shell_words parser correctly block command injection in engine commands.
• The _data_root_key() → resolve(strict=False) normalization prevents ..-traversal in model-supplied --data-root values.
• The _PLUGIN_ROOT_PLACEHOLDER check correctly treats an unsubstituted ${CLAUDE_PLUGIN_ROOT} literal as absent and fails closed.
• The plugin_id sanitisation ([^A-Za-z0-9_-] → -) prevents path traversal through marketplace or plugin-name components.
• The documented kill-switch gap (cannot deny outright in skill-hook deployment) is correctly treated as fail-safe rather than fail-open: the guard still prompts before every mutation.

Claude finished @kyle-sexton's task in 3m 32s —— View job

PR #1014 Code Review

• Gather context and understand the PR
• Read changed files and analyze the fix
• Review destructive_guard.py — derivation logic and fail-closed behavior
• Review SKILL.md hook args change
• Review test_hygiene.py — new test coverage
• Review safety-model.md docs update
• Review plugin.json version bump and CHANGELOG.md
• Post final review

Summary

This PR correctly fixes a fail-open in the clean skill's PreToolUse guard: the hook previously declared ${CLAUDE_PLUGIN_DATA} in its args, which Claude Code refuses to substitute for skill hooks, causing the hook launch to fail silently (non-blocking error). The guard never ran. The fix replaces that token with ${CLAUDE_PLUGIN_ROOT} — the only substitution a skill hook receives — and derives the authorized data root from the plugin root via the documented <plugins>/cache/<marketplace>/<name> → <plugins>/data/<id> layout. Test coverage is thorough. Documentation is honest about the known kill-switch limitation.

Overall this is a well-structured security fix with correct fail-closed behavior throughout. One testability fragility and a couple of minor notes below.

Findings

Medium — Test isolation gap: test_guard_denies_data_root_without_hook_authority does not mock sys.argv

test_hygiene.py:1363

This test constructs its own mock context but omits mock.patch.object(guard.sys, "argv", ...). Before this PR, resolve_authorized_data_root() only read --authorized-data-root from argv — a flag no standard test runner would inject. Now it also reads --plugin-root, which it then runs through _plugin_data_root_from_root. If a test runner invocation happened to contain --plugin-root <path-matching-cache/mp/name> anywhere in its argv, the derivation would succeed and the test that expects a deny could instead get allow.

In practice this is low probability (the test expects a deny and the test environment controls argv), but it's a new fragility this PR introduces. All three run_guard_* helpers correctly patch sys.argv; this test should too:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),  # add this
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

Minor — _plugin_data_root_from_root docstring could acknowledge the sanitization collision risk

destructive_guard.py:123

The @ → - sanitization in:

plugin_id = _PLUGIN_ID_DISALLOWED.sub("-", f"{name}@{marketplace}")

can produce collisions — e.g. disk@hygiene-acme and disk-hygiene@acme both yield disk-hygiene-acme. The existing test at line 1643 documents this explicitly for . in names, which is good. The docstring says <id> is <name>@<marketplace> with disallowed characters replaced by -, which is accurate. No bug — this mirrors the documented Claude Code behavior. The test coverage is sufficient; just flagging it since the derivation now has security implications.

Minor — Coupling to undocumented plugins directory name

destructive_guard.py:143

if (
    cache_dir.name.casefold() != _PLUGIN_CACHE_DIRNAME
    or plugins_dir.name.casefold() != _PLUGINS_DIRNAME
):
    return None

The check for plugins as the parent directory name is the "one undocumented part" the safety-model.md correctly flags. The fail-closed design handles this cleanly — a future Claude Code layout change yields no authority rather than a wrong one. The docstring and safety model doc both acknowledge this, so no action needed beyond what's already documented.

What's correct and well done

Core fix: The hook args change from ${CLAUDE_PLUGIN_DATA} to --plugin-root ${CLAUDE_PLUGIN_ROOT} is exactly right. SKILL.md:13

Fail-closed derivation: _plugin_data_root_from_root returning None on any unrecognized layout, which flows through to _is_authorized_data_root(value, None) → False, denying --data-root engine calls while keeping the guard fully active. Correct throughout.

Precedence chain in resolve_authorized_data_root: --authorized-data-root → --plugin-root derivation → CLAUDE_PLUGIN_DATA env. The exact-placeholder filter (!= _AUTHORIZED_DATA_ROOT_PLACEHOLDER) only rejects the literal unsubstituted token, not real paths containing ${ — the test at line 1587 explicitly covers this edge case. destructive_guard.py:196

Seam-lock tests: test_skill_hook_args_launch_with_only_skill_available_substitutions (line 1738) and test_skill_hook_passes_plugin_root_flag_matching_constant (line 1765) lock the config↔code contract so a future rename on either side doesn't silently reintroduce the fail-open. These are the right tests to have.

run_guard_plugin_root helper simulates the exact skill-hook deployment condition (no CLAUDE_PLUGIN_DATA in env, --plugin-root in argv), making the end-to-end test test_guard_derives_data_root_from_plugin_root_argv a faithful replica of the real runtime path.

Kill switch fail-safe: Defaulting to enabled = True when no channel delivers the value is the correct fail-safe behavior for the skill hook context (guard stays active and forces human prompt). The SKILL.md body correctly explains that the model — not the guard — enforces audit-only mode from its substituted disk_hygiene_enabled value. destructive_guard.py:208

Documentation accuracy: safety-model.md now honestly describes the derivation, the undocumented-layout coupling, and the kill-switch delivery gap. CHANGELOG and version bump are correct.

🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate.

Claude finished @kyle-sexton's task in 4m 15s —— View job

Security Review — PR #1014

• Gather context and understand the PR
• Read changed files via gh pr diff
• Analyze destructive_guard.py — path derivation, injection surfaces, authorization logic
• Verify whether prior findings (relative-path guard, test isolation gap, versioned-layout depth) were addressed
• Check SKILL.md hook configuration for new attack surface
• Analyze test_hygiene.py for seam-lock coverage gaps
• Post security findings

Summary

The core fix is correct and addresses a real fail-open: replacing ${CLAUDE_PLUGIN_DATA} with ${CLAUDE_PLUGIN_ROOT} in the skill hook args ensures the guard process launches. The new _plugin_data_root_from_root derivation uses a marker-scan approach (parts[i]=="cache" and parts[i-1]=="plugins") rather than fixed-depth parents[] indexing — this directly resolves the Codex P1 finding (versioned <name>/<version> layout) that was filed against commit 834f98b. That finding is resolved by commit fbf3ba5. Two prior findings from the initial security/code reviews remain open.

Findings

IMPORTANT — _plugin_data_root_from_root still accepts relative paths and returns a relative authority

Confidence: CONFIRMED — this exact finding was raised by the prior security review (comment 2026-07-22T16:09:29Z) and is NOT addressed in any commit on this branch.

destructive_guard.py line 143–157

parts = Path(plugin_root).parts
for index in range(1, len(parts)):
    if not (
        parts[index].casefold() == _PLUGIN_CACHE_DIRNAME
        and parts[index - 1].casefold() == _PLUGINS_DIRNAME
    ):
        continue
    ...
    plugins_dir = Path(*parts[:index])
    return os.fspath(plugins_dir / _PLUGIN_DATA_DIRNAME / plugin_id)

For a relative input such as plugins/cache/m/n, parts[0]="plugins" and parts[1]="cache" match at index=1. Path(*parts[:1]) = Path("plugins") — relative. The returned authority is "plugins/data/n-m" — also relative. When _is_authorized_data_root() later calls _data_root_key() → Path(value).expanduser().resolve(strict=False), both sides resolve against the guard process's cwd, meaning a crafted --data-root pointing to the same cwd-relative location would pass the check.

In the skill-hook deployment this requires ${CLAUDE_PLUGIN_ROOT} to be non-absolute (Claude Code always supplies absolute paths and the model cannot inject into hook args), so direct exploitability is low. But _plugin_data_root_from_root is a security boundary: the defence should be in code, not reliant on an external guarantee. The test suite does not cover relative inputs.

Recommended fix (same as prior review's recommendation):

def _plugin_data_root_from_root(plugin_root: str) -> str | None:
    if not os.path.isabs(plugin_root):
        return None
    ...

Add a corresponding test assertion alongside the existing stray list in test_plugin_data_root_from_root_follows_documented_layout:

self.assertIsNone(guard._plugin_data_root_from_root("plugins/cache/m/n"))

Fix this →

SUGGESTION — test_guard_denies_data_root_without_hook_authority does not mock sys.argv

Confidence: CONFIRMED — this exact finding was raised by the prior code review (comment 2026-07-22T16:09:32Z) and is NOT addressed in any commit on this branch.

test_hygiene.py line 1583–1588

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.dict("os.environ", environment, clear=True),  # no sys.argv patch
):
    self.assertEqual(0, guard.main())

Since resolve_authorized_data_root() now also reads --plugin-root from sys.argv, a test runner invocation containing --plugin-root <path-matching-cache/mp/name> in its real argv could cause the derivation to succeed, producing an allow where the test expects deny. All other run_guard_* helpers correctly patch sys.argv; this one does not. Low probability in practice, but it is a new fragility this PR introduces relative to the old code.

Recommended fix:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

What is verified correct

• Codex P1 resolved: The versioned-layout depth issue (parents[1] off by one for installed <name>/<version> roots) was the old 834f98b approach. Commit fbf3ba5 ("fix(disk-hygiene): derive data root from real versioned install layout") replaced it with the parts marker-scan that correctly handles both <name>/<version> and direct-link <name> roots. The Codex finding does not apply to the current code.
• Hook args are not model-injectable: The new hook passes only ["…/destructive_guard.py", "--plugin-root", "${CLAUDE_PLUGIN_ROOT}"]. Both are harness-substituted; the model has no injection surface.
• Fail-closed on unrecognized layout: _plugin_data_root_from_root returning None → _is_authorized_data_root(value, None) → False. Engine calls with --data-root are denied while the guard stays fully active.
• No new injection surfaces in _argv_flag_value: reads only from sys.argv, which in skill-hook deployment is harness-controlled.
• Seam-lock tests are correct: test_skill_hook_args_launch_with_only_skill_available_substitutions and test_skill_hook_passes_plugin_root_flag_matching_constant lock the config↔code contract; a rename on either side will fail the test.
• run_guard_plugin_root helper faithfully replicates skill-hook conditions (no CLAUDE_PLUGIN_DATA in env, --plugin-root in argv only).

Claude finished @kyle-sexton's task in 3m 31s —— View job

PR #1014 Review

• Gather context
• Read changed files
• Analyze Codex P1 finding: versioned cache path depth
• Verify prior security findings (relative path guard, sys.argv mock)
• Post final review

Codex P1: Versioned cache path — Resolved

The Codex review was based on commit 834f98b, which used fixed-depth parents[0/1/2] indexing that would fail for versioned installs (cache/<mkt>/<name>/<version>). Commit fbf3ba5 already fixes this by replacing fixed depth with an index-scanning approach that iterates Path.parts looking for the plugins/cache marker:

parts = Path(plugin_root).parts
for index in range(1, len(parts)):
    if not (
        parts[index].casefold() == _PLUGIN_CACHE_DIRNAME
        and parts[index - 1].casefold() == _PLUGINS_DIRNAME
    ):
        continue
    marketplace, name = parts[index + 1], parts[index + 2]

For a versioned install at …/plugins/cache/melodic-software/disk-hygiene/0.4.6, the loop finds cache at its correct position, takes parts[index+1] (marketplace) and parts[index+2] (name), and the version leaf is silently ignored — exactly the documented behaviour. test_plugin_data_root_from_root_follows_documented_layout now covers both the versioned and unversioned layouts explicitly (lines 1841–1856). The Codex P1 does not apply to the current branch tip.

Still-open: Relative path not rejected in _plugin_data_root_from_root

The previous security review's IMPORTANT finding remains unresolved. _plugin_data_root_from_root has no early return for non-absolute inputs:

destructive_guard.py:143

A relative input like plugins/cache/m/n satisfies all layout checks (parts[1]="cache" under parts[0]="plugins") and returns plugins/data/n-m — a relative path. When _is_authorized_data_root later calls _data_root_key() → resolve(strict=False) on that relative authority, it anchors to the guard process's cwd rather than any intentional location. A model-supplied --data-root plugins/data/n-m would then match and be authorized.

Exploitability in this deployment is very low — Claude Code always provides an absolute ${CLAUDE_PLUGIN_ROOT}, and the model cannot inject into hook args — but _plugin_data_root_from_root is a security boundary and should not depend on an external runtime guarantee. The test suite at line 1867 covers stray absolute paths but has no relative-path case.

Recommended fix (two lines):

def _plugin_data_root_from_root(plugin_root: str) -> str | None:
    if not os.path.isabs(plugin_root):   # add this
        return None                       # add this
    parts = Path(plugin_root).parts
    ...

And add one assertion to the stray list in test_plugin_data_root_from_root_follows_documented_layout:

"plugins/cache/m/n",   # relative path must fail closed

Fix this →

Still-open: test_guard_denies_data_root_without_hook_authority does not patch sys.argv

test_hygiene.py:1583

The test constructs its own mock context but omits mock.patch.object(guard.sys, "argv", ...). The guard now reads --plugin-root from sys.argv (in addition to --authorized-data-root). If a test runner happened to pass --plugin-root <path-matching-cache/mkt/name>, the derivation would succeed and the test that expects deny would get allow instead. Low probability in practice, but it's a fragility this PR introduced. All three run_guard_* helpers correctly patch sys.argv; this test should too:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),  # add
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

What's correct and well done

• Core fix is sound. The hook args change from ${CLAUDE_PLUGIN_DATA} → --plugin-root ${CLAUDE_PLUGIN_ROOT} is the right fix and the seam-lock tests (test_skill_hook_args_launch_with_only_skill_available_substitutions, test_skill_hook_passes_plugin_root_flag_matching_constant) confirm the config↔code contract statically.
• Versioned layout derivation (added in fbf3ba5) correctly handles the <version> leaf and the no-version local-install case with a single index-scan loop.
• Fail-closed derivation — _plugin_data_root_from_root returning None on unrecognized layout flows correctly through to deny on --data-root calls while keeping the guard active.
• run_guard_plugin_root helper simulates the exact skill-hook condition (no CLAUDE_PLUGIN_DATA in env, --plugin-root in argv) making test_guard_derives_data_root_from_plugin_root_argv a faithful integration replica of the real runtime path.
• Kill switch fail-safe — defaulting enabled = True when no channel delivers the value is the correct behaviour for a skill-hook deployment; the model still enforces audit-only mode from its substituted disk_hygiene_enabled value.
• Documentation — SKILL.md and safety-model.md are accurate about the derivation, the undocumented-layout coupling, and the kill-switch delivery gap.

Claude finished @kyle-sexton's task in 2m 18s —— View job

PR #1014 Review

• Gather context
• Read destructive_guard.py — check if relative-path guard was added
• Read test_hygiene.py — check if sys.argv mock was added
• Review safety-model.md — assess new docs commit vs Codex P2
• Post final review

Status after latest commit (d8b32b8)

The latest commit is docs-only (safety-model.md). The two findings from prior security and code reviews remain unaddressed in code.

Still open: _plugin_data_root_from_root accepts relative paths

IMPORTANT — Confidence: CONFIRMED — raised in all three prior reviews; no commit has addressed it.

destructive_guard.py:143

parts = Path(plugin_root).parts
for index in range(1, len(parts)):
    if not (
        parts[index].casefold() == _PLUGIN_CACHE_DIRNAME
        and parts[index - 1].casefold() == _PLUGINS_DIRNAME
    ):
        continue
    ...
    plugins_dir = Path(*parts[:index])
    return os.fspath(plugins_dir / _PLUGIN_DATA_DIRNAME / plugin_id)

For a relative input plugins/cache/m/n, parts[0]="plugins" and parts[1]="cache" match at index=1. Path(*parts[:1]) = Path("plugins") — relative. The returned authority is "plugins/data/n-m" — also relative. When _is_authorized_data_root() later calls _data_root_key() → resolve(strict=False), both sides anchor to the guard process's cwd, so a crafted --data-root plugins/data/n-m would match and be authorized.

Exploitability is low given Claude Code always provides an absolute ${CLAUDE_PLUGIN_ROOT} and the model cannot inject into hook args. But _plugin_data_root_from_root is a security boundary and must not rely on an external runtime guarantee. The stray test list at line 1867 covers only absolute mismatches — no relative-path case is present.

Fix (two lines of code + one test assertion):

def _plugin_data_root_from_root(plugin_root: str) -> str | None:
    if not os.path.isabs(plugin_root):
        return None
    parts = Path(plugin_root).parts
    ...

And add "plugins/cache/m/n" to the stray list in test_plugin_data_root_from_root_follows_documented_layout.

Fix this →

Still open: test_guard_denies_data_root_without_hook_authority does not patch sys.argv

SUGGESTION — Confidence: CONFIRMED — raised in all three prior reviews; no commit has addressed it.

test_hygiene.py:1583

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.dict("os.environ", environment, clear=True),  # no sys.argv patch
):
    self.assertEqual(0, guard.main())
result = json.loads(stdout.getvalue())
self.assertEqual("deny", result["hookSpecificOutput"]["permissionDecision"])

resolve_authorized_data_root() now reads --plugin-root from sys.argv in addition to --authorized-data-root. The test correctly excludes CLAUDE_PLUGIN_DATA from the environment, but if the test runner's real sys.argv happened to contain --plugin-root <absolute-path-with-cache/mp/name>, the derivation would succeed and the test that expects "deny" would get "allow" instead — or vice-versa if the derived path happened to match the temp directory. All three run_guard_* helpers patch sys.argv; this test should too:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

Codex P2 (--plugin-dir local dev) — Addressed by docs

d8b32b8 documents in safety-model.md that a claude --plugin-dir <checkout> session has no <plugins>/cache/<marketplace> structure and therefore no derivable authority. The guard's fail-closed behaviour in that mode is correctly described as a deliberate safe-over-convenient tradeoff for dev-only use, not a security gap. The Codex P2 finding is resolved by documentation; no code change is needed for that scenario.

What remains correct

• Core fix (hook args): ${CLAUDE_PLUGIN_DATA} → --plugin-root ${CLAUDE_PLUGIN_ROOT} is sound. Seam-lock tests confirm the config↔code contract.
• Fail-closed derivation: _plugin_data_root_from_root returning None on unrecognized layout flows correctly to deny while keeping the guard active.
• Index-scan for versioned layout: The marker-scan in fbf3ba5 correctly handles both <name>/<version> and no-version roots; the Codex P1 finding does not apply to the current code.
• Kill switch fail-safe: Defaulting enabled = True when no channel delivers the value is the correct behavior in skill-hook context.
• Documentation: safety-model.md now accurately describes derivation, undocumented-layout coupling, kill-switch delivery gap, and the --plugin-dir dev mode limitation.

The two open findings above are the only unresolved items from the full review cycle.

Claude finished @kyle-sexton's task in 5m 2s —— View job

Security Review — PR #1014

• Gather PR diff and read changed files
• Check whether prior unresolved findings were addressed in latest commit (d8b32b8 — docs-only)
• Analyze destructive_guard.py for new or remaining security issues
• Post findings

Summary

The latest commit (d8b32b8) is documentation-only (safety-model.md). The two security findings raised by every prior review round remain unaddressed in the current branch tip. One new defence-in-depth gap is noted in the is_exact_kill_switch_probe function that arrived via the merge from origin/main.

IMPORTANT — _plugin_data_root_from_root still accepts relative paths

Severity: IMPORTANT
Confidence: CONFIRMED — the missing check is directly observable in the code; exploitability requires a non-absolute ${CLAUDE_PLUGIN_ROOT}

destructive_guard.py:143

The function has no absolute-path guard. For a relative input such as plugins/cache/m/n:

• parts = ("plugins", "cache", "m", "n") — the marker scan matches at index=1
• Path(*parts[:1]) → Path("plugins") — relative
• Returns "plugins/data/n-m" — also relative

When _is_authorized_data_root() later canonicalises this via _data_root_key() → resolve(strict=False), both the derived authority and a crafted --data-root plugins/data/n-m resolve against the guard process's cwd. A model-supplied --data-root pointing to the same cwd-relative location would then match and be authorized.

In the current skill-hook deployment ${CLAUDE_PLUGIN_ROOT} is always absolute (Claude Code guarantees it and the model cannot inject into hook args), so direct exploitability is low — but _plugin_data_root_from_root is a security boundary that should not rely on an external runtime guarantee.

This finding was raised in the first security review (2026-07-22T16:09:29Z), repeated in the second (2026-07-22T17:07:07Z), and again in the third. It remains unaddressed in all five commits pushed since then.

Recommended fix — two lines at the top of _plugin_data_root_from_root:

if not os.path.isabs(plugin_root):
    return None

And add one entry to the stray list in test_plugin_data_root_from_root_follows_documented_layout:

"plugins/cache/m/n",   # relative path must fail closed

Fix this →

SUGGESTION — test_guard_denies_data_root_without_hook_authority does not mock sys.argv

Severity: SUGGESTION
Confidence: CONFIRMED — the missing patch is directly observable

test_hygiene.py:1583

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.dict("os.environ", environment, clear=True),  # no sys.argv patch
):

resolve_authorized_data_root() now reads --plugin-root from sys.argv in addition to --authorized-data-root. If a test runner passed --plugin-root /x/plugins/cache/m/n in its real argv, the derivation would succeed and the test that expects deny would receive allow. All three run_guard_* helpers correctly patch sys.argv; this test should too. Raised in the first code review (2026-07-22T16:09:32Z) and again in the second. Remains unaddressed.

Recommended fix:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

SUGGESTION — is_exact_kill_switch_probe fails open when kill_switch_probe.py is absent from disk

Severity: SUGGESTION
Confidence: CONFIRMED — behaviour is directly observable from _script_path_key's contract; exploitability requires the script file to be missing

destructive_guard.py:361

def is_exact_kill_switch_probe(command: str) -> bool:
    ...
    expected_script = str(
        Path(__file__).resolve().parents[2] / "setup" / "scripts" / "kill_switch_probe.py"
    )
    return _script_path_key(tokens[1]) == _script_path_key(expected_script)

_script_path_key calls Path(value).resolve(strict=True) and returns None on OSError. When kill_switch_probe.py doesn't exist on disk, _script_path_key(expected_script) returns None. Any model-supplied tokens[1] that also doesn't exist returns None, and None == None is True — the function grants "allow" for the command.

Constraints limit harm: the command must be exactly 2 tokens with no shell expansion (_literal_shell_words rejects all operator/expansion characters); tokens[0] must be the real Python executable validated by os.path.samefile; and the allowed command (python <nonexistent>) fails at execution anyway. Exploiting this to bypass a deny verdict requires the model to know the exact Python executable path and kill_switch_probe.py to be absent — unusual for a working install. The pre-existing classify_exact_engine_command carries the same None != None pattern for hygiene.py (prior reviews accepted this implicitly).

Recommended fix: add an explicit existence check before the comparison:

expected_key = _script_path_key(expected_script)
if expected_key is None:
    return False  # script absent — fail closed rather than None == None
return _script_path_key(tokens[1]) == expected_key

What is verified correct at HEAD

• Core fix is sound: replacing ${CLAUDE_PLUGIN_DATA} with --plugin-root ${CLAUDE_PLUGIN_ROOT} in SKILL.md ensures the guard process launches. The seam-lock tests lock the config↔code contract.
• Versioned-layout derivation (commit fbf3ba5): the index-scan over Path.parts correctly handles both <name>/<version> and bare <name> install roots; the Codex P1 finding from 834f98b does not apply to the current code.
• Fail-closed on unrecognised layout: _plugin_data_root_from_root returning None → _is_authorized_data_root(value, None) → False. Engine calls with --data-root are denied while the destructive-action guard stays active.
• No new injection surfaces: _literal_shell_words rejects all shell expansion and operator characters; _data_root_key() → resolve(strict=False) prevents ..-traversal in model-supplied paths; _PLUGIN_ID_DISALLOWED sanitisation prevents path traversal through marketplace/name components.
• --plugin-dir local-dev gap documented (d8b32b8): correctly characterised in safety-model.md as a deliberate safe-over-convenient tradeoff, not a security gap. The Codex P2 finding is addressed by documentation.
• Kill-switch fail-safe: defaulting enabled = True when no channel delivers the value is the correct fail-safe behaviour for a skill-hook deployment.

Claude finished @kyle-sexton's task in 3m 12s —— View job

PR #1014 Review

• Gather context and understand the PR
• Read latest changed files (HEAD: 6f05652)
• Check whether persistent open findings were addressed
• Check Codex P2 finding (docs accuracy for kill switch)
• Post final review

Status after HEAD (6f05652 — docs-only commit)

The latest commit correctly updates README.md and kill_switch_probe.py to reflect that audit-only mode is model-enforced in the skill-hook deployment, not guard-enforced. That resolves the Codex P2 finding. The two code-level findings raised in every prior review round remain unaddressed.

IMPORTANT — _plugin_data_root_from_root still accepts relative paths (round 4)

Confidence: CONFIRMED — the missing check is directly observable; raised in all four prior reviews, unaddressed across all six commits on this branch.

destructive_guard.py:143

parts = Path(plugin_root).parts       # no absoluteness guard here
for index in range(1, len(parts)):
    if not (
        parts[index].casefold() == _PLUGIN_CACHE_DIRNAME
        and parts[index - 1].casefold() == _PLUGINS_DIRNAME
    ):
        continue
    ...
    plugins_dir = Path(*parts[:index])  # relative if plugin_root was relative
    return os.fspath(plugins_dir / _PLUGIN_DATA_DIRNAME / plugin_id)  # relative authority

For a relative input plugins/cache/m/n, parts[0]="plugins" and parts[1]="cache" match at index=1. Path(*parts[:1]) = Path("plugins") — relative. The returned authority is "plugins/data/n-m" — also relative. When _is_authorized_data_root() later calls _data_root_key() → resolve(strict=False), both sides anchor to the guard process's cwd; a crafted --data-root plugins/data/n-m would match and be authorized.

Exploitability in deployment is low — Claude Code guarantees an absolute ${CLAUDE_PLUGIN_ROOT} and the model cannot inject into hook args — but _plugin_data_root_from_root is a security boundary. The stray-path test list at line 1867 covers only absolute mismatches; no relative-path case is present.

Fix — two lines of code:

def _plugin_data_root_from_root(plugin_root: str) -> str | None:
    if not os.path.isabs(plugin_root):  # add
        return None                      # add
    parts = Path(plugin_root).parts
    ...

And add one entry to the stray tuple in test_plugin_data_root_from_root_follows_documented_layout at line 1867:

"plugins/cache/m/n",   # relative path must fail closed

Fix this →

SUGGESTION — test_guard_denies_data_root_without_hook_authority does not patch sys.argv (round 3)

Confidence: CONFIRMED — raised in all three prior code-review rounds, unaddressed.

test_hygiene.py:1583

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.dict("os.environ", environment, clear=True),  # no sys.argv patch
):
    self.assertEqual(0, guard.main())

resolve_authorized_data_root() now reads --plugin-root from sys.argv in addition to --authorized-data-root. If a test runner passed --plugin-root /x/plugins/cache/m/n in real argv, the derivation would succeed and the expected "deny" would become "allow". All three run_guard_* helpers correctly patch sys.argv; this test should too:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

SUGGESTION — is_exact_kill_switch_probe fails open when kill_switch_probe.py is absent from disk (carried from prior review)

Confidence: CONFIRMED — raised in the 2026-07-22T18:13:30Z security review; unaddressed.

destructive_guard.py:361

expected_script = str(
    Path(__file__).resolve().parents[2] / "setup" / "scripts" / "kill_switch_probe.py"
)
return _script_path_key(tokens[1]) == _script_path_key(expected_script)

_script_path_key calls resolve(strict=True) and returns None on OSError. When kill_switch_probe.py is absent from disk, _script_path_key(expected_script) = None; any model-supplied tokens[1] that also fails resolve returns None, and None == None is True — granting an allow incorrectly. Harm is constrained: the command must be exactly 2 tokens with no shell expansion, and tokens[0] must be the real Python executable; but the boundary should be explicit. Recommended fix:

expected_key = _script_path_key(expected_script)
if expected_key is None:
    return False   # script absent — fail closed rather than None == None
return _script_path_key(tokens[1]) == expected_key

Codex P2 — Resolved ✓

Commit 6f05652 correctly updates both README.md (security-review bullet at lines 139–147) and kill_switch_probe.py (module docstring at lines 12–19) to reflect that audit-only mode is model-enforced (via the probe) rather than guard-enforced. The guard's actual guarantee — forcing a human prompt before every mutation — is preserved and accurately stated in both places. No further docs fix is needed for this finding.

What remains correct and well done

• Core fix is sound. Hook args ${CLAUDE_PLUGIN_DATA} → --plugin-root ${CLAUDE_PLUGIN_ROOT} ensures the guard process launches. Seam-lock tests (test_skill_hook_args_launch_with_only_skill_available_substitutions, test_skill_hook_passes_plugin_root_flag_matching_constant) lock the config↔code contract.
• Versioned-layout derivation (fbf3ba5) correctly handles both <name>/<version> and bare <name> roots via index-scan; Codex P1 does not apply to current code.
• Fail-closed on unrecognized layout: _plugin_data_root_from_root returning None → _is_authorized_data_root(value, None) → False. Engine --data-root calls are denied while the guard stays active.
• run_guard_plugin_root helper faithfully replicates skill-hook conditions (no CLAUDE_PLUGIN_DATA in env, --plugin-root in argv only), making test_guard_derives_data_root_from_plugin_root_argv a faithful end-to-end replica of the real runtime path.
• Kill switch fail-safe: defaulting enabled = True when no channel delivers the value is the correct behaviour; the model still enforces audit-only mode from the probe result.
• Documentation is now accurate end-to-end: safety-model.md, SKILL.md, README.md, and kill_switch_probe.py all correctly describe the derivation, undocumented-layout coupling, and kill-switch delivery gap.

The one IMPORTANT (relative-path absoluteness guard) and two SUGGESTION findings above are the only unresolved items across the full review cycle. The IMPORTANT finding is a two-line code change that closes a defence-in-depth gap in a security boundary; it is the only item that should block merge.

Claude finished @kyle-sexton's task in 3m 32s —— View job

Security Review — PR #1014

• Gather PR diff and identify changed files at HEAD (6f05652)
• Read destructive_guard.py — check if relative-path guard was added
• Read test_hygiene.py — check if sys.argv mock was added
• Check kill_switch_probe.py and is_exact_kill_switch_probe — assess None == None finding
• Review SKILL.md, README.md, docs for Codex P2 kill-switch doc accuracy
• Post security findings

Summary

HEAD is 6f05652 (docs-only: corrects stale audit-only enforcement claims in README and kill_switch_probe.py). Two prior findings remain unresolved in code. One pre-existing None == None defect in is_exact_kill_switch_probe carries only theoretical risk but is a correctness issue in a security boundary.

IMPORTANT — _plugin_data_root_from_root still accepts relative paths

Confidence: CONFIRMED — the missing check is directly observable at HEAD; no commit in this branch has addressed it despite being raised in every prior review round.

destructive_guard.py:143

parts = Path(plugin_root).parts
for index in range(1, len(parts)):
    if not (
        parts[index].casefold() == _PLUGIN_CACHE_DIRNAME
        and parts[index - 1].casefold() == _PLUGINS_DIRNAME
    ):
        continue
    ...
    plugins_dir = Path(*parts[:index])
    return os.fspath(plugins_dir / _PLUGIN_DATA_DIRNAME / plugin_id)

For a relative input such as plugins/cache/m/n, parts[0]="plugins" and parts[1]="cache" match at index=1. Path(*parts[:1]) = Path("plugins") — relative. The returned authority is "plugins/data/n-m" — also relative. When _is_authorized_data_root() later calls _data_root_key() → resolve(strict=False), both the derived authority and a model-supplied --data-root plugins/data/n-m resolve against the guard process's cwd, so the check would match and the call would be authorized.

Exploitability requires ${CLAUDE_PLUGIN_ROOT} to be non-absolute; Claude Code always provides absolute paths and the model cannot inject into hook args. But _plugin_data_root_from_root is a security boundary whose correctness must not rely on that external runtime guarantee. The stray test list at line 1867 covers absolute mismatches only — no relative-path case is present.

Fix (two lines + one test assertion):

def _plugin_data_root_from_root(plugin_root: str) -> str | None:
    if not os.path.isabs(plugin_root):
        return None
    parts = Path(plugin_root).parts
    ...

Add "plugins/cache/m/n" to the stray tuple in test_plugin_data_root_from_root_follows_documented_layout.

Fix this →

SUGGESTION — test_guard_denies_data_root_without_hook_authority still does not mock sys.argv

Confidence: CONFIRMED — the missing patch is directly observable at HEAD; raised in every prior review round.

test_hygiene.py:1583

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.dict("os.environ", environment, clear=True),  # no sys.argv patch
):
    self.assertEqual(0, guard.main())

resolve_authorized_data_root() now reads --plugin-root from sys.argv. If a test runner passed --plugin-root /x/plugins/cache/m/n in its real argv, the derivation would succeed and the test that expects deny would receive allow. All three run_guard_* helpers correctly patch sys.argv; this test should too:

with (
    mock.patch("sys.stdin", stdin),
    redirect_stdout(stdout),
    mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),
    mock.patch.dict("os.environ", environment, clear=True),
):

Fix this →

SUGGESTION — is_exact_kill_switch_probe fails open when kill_switch_probe.py is absent and a nonexistent path is supplied

Confidence: CONFIRMED — the None == None case is directly observable; practical impact is bounded because the allowed command fails at execution.

destructive_guard.py:375

return _script_path_key(tokens[1]) == _script_path_key(expected_script)

_script_path_key returns None on OSError (file absent). When kill_switch_probe.py does not exist on disk, _script_path_key(expected_script) returns None. A model-supplied tokens[1] pointing to any nonexistent path also returns None, so None == None → True — the function returns True and the guard emits "allow" for a "kill-switch probe" that is in fact python <nonexistent-path>.

Practical impact is limited: the attack requires kill_switch_probe.py to be absent (degraded install) AND the model to supply a nonexistent script path (a model-supplied path that exists would return a real key, not None, making the comparison fail). The "allowed" command fails at execution regardless. However, this is a correctness error in a security boundary that mirrors the same pattern in classify_exact_engine_command for hygiene.py.

Recommended fix:

def is_exact_kill_switch_probe(command: str) -> bool:
    ...
    expected_key = _script_path_key(expected_script)
    if expected_key is None:
        return False  # script absent — fail closed, not None == None
    return _script_path_key(tokens[1]) == expected_key

Fix this →

What is resolved at HEAD

• Codex P1 (versioned layout depth): fbf3ba5 replaced fixed-depth parents[] with an index-scan over Path.parts that correctly handles <name>/<version> and bare <name> roots. Confirmed resolved.
• Codex P2 (--plugin-dir local dev): d8b32b8 documents in safety-model.md that a claude --plugin-dir <checkout> session lacks <plugins>/cache/<marketplace> structure, so the derivation yields no authority and the engine lane is deliberately fail-closed in that dev-only mode. Confirmed resolved.
• Codex P2 (kill-switch doc accuracy): 6f05652 corrects both the README.md security-review bullet and the kill_switch_probe.py docstring to state that audit-only mode is model-enforced (via the probe), not guard-enforced. Both now accurately describe the current behavior. Confirmed resolved.
• Core fix: The hook args change from ${CLAUDE_PLUGIN_DATA} → --plugin-root ${CLAUDE_PLUGIN_ROOT} is sound. Seam-lock tests lock the config↔code contract.
• Fail-closed derivation: _plugin_data_root_from_root returning None on unrecognized layout flows correctly to deny on --data-root calls while keeping the guard active.
• No injection surfaces in _literal_shell_words: correctly blocks all shell expansion/operator characters; _data_root_key() → resolve(strict=False) prevents ..-traversal in model-supplied paths; _PLUGIN_ID_DISALLOWED sanitisation prevents path traversal through marketplace/name components.